Skip to content

Pass -D even for default values#4354

Merged
wujingyue merged 5 commits intomainfrom
wjy/option
May 2, 2025
Merged

Pass -D even for default values#4354
wujingyue merged 5 commits intomainfrom
wjy/option

Conversation

@wujingyue
Copy link
Collaborator

@wujingyue wujingyue commented Apr 30, 2025

This is a follow-up to #4319

Before this PR, we don't pass -D for default values. This confused me
several times, so I'm making this change to ensure consistent behavior
across clean and dirty builds. Refer to code comments for details.

@wujingyue
Pass -D even for default values.
8c0a95b 
A follow-up to #4319

Before this PR, we don't pass -D for default values. This confused me
several times, so I'm making this change to ensure consistent behavior
across clean and dirty builds. Refer to code comments for details.
@wujingyue
Comment
72d539a
@wujingyue
Copy link
Collaborator Author

wujingyue commented Apr 30, 2025

!test

@wujingyue wujingyue requested a review from jjsjann123 April 30, 2025 22:11
@github-actions
Copy link

github-actions bot commented Apr 30, 2025 •
edited
Loading

Review updated until commit 65c165e

Description

  • Always pass -D for default values in CMake options

  • Ensure consistent behavior across clean and dirty builds

  • Update python/utils.py to include all options explicitly

Changes walkthrough 📝

Relevant files
Enhancement
utils.py
Always pass -D for CMake options                                                 

python/utils.py

  • Added comments explaining the rationale for always passing -D for
    CMake options
  • Updated cmd_str to include all CMake options explicitly, removing
    conditional appends
    		• +17/-16 

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Code Duplication

    The new code introduces a lot of repeated lines that were previously conditional. This could lead to maintenance issues and potential inconsistencies.

    f"-DCMAKE_BUILD_TYPE={config.build_type}",
f"-DCMAKE_INSTALL_PREFIX={install_prefix}",
f"-DNVFUSER_CPP_STANDARD={config.cpp_standard}",
f"-DUSE_DISTRIBUTED={pytorch_use_distributed}",
f"-DNVFUSER_BUILD_WITH_ASAN={on_or_off(config.build_with_asan)}",
f"-DNVFUSER_STANDALONE_BUILD_WITH_UCC={on_or_off(config.build_with_ucc)}",
f"-DNVFUSER_EXPLICIT_ERROR_CHECK={on_or_off(config.explicit_error_check)}",
f"-DBUILD_TEST={on_or_off(not config.no_test)}",
f"-DBUILD_PYTHON={on_or_off(not config.no_python)}",
f"-DPython_EXECUTABLE={sys.executable}",
f"-DBUILD_NVFUSER_BENCHMARK={on_or_off(not config.no_benchmark)}",
f"-DNVFUSER_DISTRIBUTED={on_or_off(not config.build_without_distributed)}",
f"-DUSE_SYSTEM_NVTX={on_or_off(config.build_with_system_nvtx)}",
"-B",
    Comment Clarity

    The comment about cmake options being sticky is helpful, but it could be more concise and focused on the specific changes made in this PR.

    # cmake options are sticky: when -DFOO=... isn't specified, cmake's option
# FOO prefers the cached value over the default value. This behavior
# confused me several times (e.g.
# https://github.com/NVIDIA/Fuser/pull/4319) when I ran `pip install -e`,
# so I chose to always pass these options even for default values. Doing so
# explicitly overrides cached values and ensures consistent behavior across
    CMake Option Order

    The order of CMake options has changed. Ensure that this order does not affect the behavior of CMake or the build process.

    f"-DCMAKE_BUILD_TYPE={config.build_type}",
f"-DCMAKE_INSTALL_PREFIX={install_prefix}",
f"-DNVFUSER_CPP_STANDARD={config.cpp_standard}",
f"-DUSE_DISTRIBUTED={pytorch_use_distributed}",
f"-DNVFUSER_BUILD_WITH_ASAN={on_or_off(config.build_with_asan)}",
f"-DNVFUSER_STANDALONE_BUILD_WITH_UCC={on_or_off(config.build_with_ucc)}",
f"-DNVFUSER_EXPLICIT_ERROR_CHECK={on_or_off(config.explicit_error_check)}",
f"-DBUILD_TEST={on_or_off(not config.no_test)}",
f"-DBUILD_PYTHON={on_or_off(not config.no_python)}",
f"-DPython_EXECUTABLE={sys.executable}",
f"-DBUILD_NVFUSER_BENCHMARK={on_or_off(not config.no_benchmark)}",
f"-DNVFUSER_DISTRIBUTED={on_or_off(not config.build_without_distributed)}",
f"-DUSE_SYSTEM_NVTX={on_or_off(config.build_with_system_nvtx)}",
"-B",

    wujingyue
    wujingyue commented May 1, 2025
    View reviewed changes
    @wujingyue
    Copy link
    Collaborator Author

    wujingyue commented May 1, 2025

    !test

    @wujingyue
    Copy link
    Collaborator Author

    wujingyue commented May 1, 2025

    !test

    jjsjann123
    jjsjann123 approved these changes May 2, 2025
    View reviewed changes
    Copy link
    Collaborator

    @jjsjann123 jjsjann123 left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    :shipit:

    @wujingyue
    Copy link
    Collaborator Author

    wujingyue commented May 2, 2025

    !test

    @wujingyue wujingyue merged commit 5c0fb06 into main May 2, 2025
    52 of 53 checks passed
    @wujingyue wujingyue deleted the wjy/option branch May 2, 2025 23:07
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @jjsjann123 jjsjann123 jjsjann123 approved these changes

    Assignees

    No one assigned

    Labels

    build

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @wujingyue @jjsjann123